-
Notifications
You must be signed in to change notification settings - Fork 734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL] Move SYCL Module Splitting to library. Part 2 #13282
[SYCL] Move SYCL Module Splitting to library. Part 2 #13282
Conversation
Hi Maksim, Do you want me to start reviewing or wait for it to come out of draft state? Thanks |
@asudarsa I am going to make it open in an hour. |
Awesome! Thanks. Also, I do understand the basic premise of this change. But, a few lines in the description about the purpose and scope of this change. |
Add SYCL Module Splitting as a library. ESIMD splitting is not present in this patch and will be added in the upcoming patch. Add particular testing tool sycl-module-split that invokes added functionality. Copy the following tests from llvm/test/tools/sycl-post-link/device-code-split: auto-module-split-1.ll auto-module-split-2.ll auto-module-split-3.ll auto-module-split-func-ptr.ll basic-module-split.ll complex-indirect-call-chain.ll one-kernel-per-module.ll per-aspect-split-1.ll per-aspect-split-2.ll per-aspect-split-3.ll per-joint-matrix-1.ll per-joint-matrix-2.ll per-joint-matrix-mad-1.ll per-joint-matrix-mad-2.ll per-joint-matrix-mad-4.ll per-joint-matrix-mad-5.ll per-reqd-sub-group-size-split-1.ll per-reqd-sub-group-size-split-2.ll per-reqd-wg-size-split-1.ll per-reqd-wg-size-split-2.ll split-with-kernel-declarations.ll vtable.ll The above tests were adjusted to use sycl-module-split tool. The following list of tests wasn't copied in this patch: per-joint-matrix-3.ll per-aspect-split-4.ll per-joint-matrix-mad-3.ll per-reqd-wg-size-split-3.ll split-with-func-ptrs.ll The above tests are exclude because they don't test module splitting itself. They will be migrated in the upcoming patch. split-with-func-ptrs.ll requires ESIMD splitting and will be migrated with the corresponding patch.
2ce8022
to
7f07cb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed llvm/lib/SYCLLowerIR/CMakeLists.txt from ESIMD perspective and LGTM
Update: I removed tests that I added initially. Instead of them I started using existing |
Hi @maksimsab Thanks for this change. There is one high level question. How will this tool (which eventually will be present as an API) fit into the overall compilation scheme. Can you please provide a few lines? I suppose you are going to attempt that in an upcoming PR? |
@@ -270,6 +273,33 @@ void dumpEntryPoints(const Module &M, bool OnlyKernelsAreEntryPoints = false, | |||
const char *msg = "", int Tab = 0); | |||
#endif // NDEBUG | |||
|
|||
struct SplitModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this new data structure? Can we not continue to use IrPropSymFilenameTriple?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possible.
This is tool only for LIT testing. In ClangLinkerWrapper only API calls will be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me. Couple of nits about using new data structures.
Please provide a couple of lines about how this will fit inside the bigger sycl-post-link functionality before approving.
Good work in incremental changes.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
Hi @jzc Can you please take a look when convenient? Thanks much. |
@maksimsab, please, expand PR description with the justification for adding a new tool instead of using standard tool for testing LLVM passes (i.e. |
BTW, there is already existing LLVM tool with similar functionality - llvm-extract. It's worth to consider integrating SYCL module splitting to this tool. |
|
My understanding is that these tools are added as temporary ‘placeholders’ as we move towards implementation the whole ‘SYCL-post-link’ functionality using API calls. So, I am not sure if we need to spend time to improve/ replace these tools. Maksim, please correct if I am mistaken. Thanks |
@asudarsa I see it as a long living tool that is used only for LIT testing because it would be difficult to test splitting stuff using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maksimsab, if we still plan to proceed with the patch, could you please make a merge with sycl
branch so that we re-trigger testing on top of the latest changes?
I would also look into d7c3713 to see if we can refactor our code so that it is testable by existing llvm-split
tool. If that requires a bigger refactoring (because we don't have target machine for our SYCL targets), then I think it is ok to proceed with our own temporary solution in form of a custom tool to test this code in isolation. But if we proceed with the patch as-is, we should make sure to have a tracker submitted to refactor everything to align everything with the upstream approach
@@ -733,6 +737,14 @@ void EntryPointGroup::rebuild(const Module &M) { | |||
Functions.insert(const_cast<Function *>(&F)); | |||
} | |||
|
|||
std::string ModuleDesc::makeSymbolTable() const { | |||
std::string ST; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm::SmallString
would be a better fit to reduce amount of re-allocations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The total size of a string is rarely small to benefit from small string optimizations. C++ mangled names are very long. What size would you suggest for SmallString for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not that it would fit into a pre-allocated area on the stack, it is that SmallString
is a wrapper around SmallVector
, which does not fully re-allocate on every +=
operation, because its capacity grows at a different pace similar to std::vector
's push_back
Thanks for the comment @AlexeySachkov. We do intend to proceed with this patch as an incremental change. @maksimsab, please merge with sycl TIP and retest. We can merge once testing is complete. I had a look at the PR you mentioned and I think we should be able to use our SPIR-V backend for this purpose and it should be pretty straightforward to shoehorn llvm-split tool for our purposes. We will open a tracker for that. Sincerely |
|
The commit seems to have broken shared libraries build: https://github.com/intel/llvm/actions/runs/9417067694/job/25941497305 I'm looking into it |
#14094 should fix post-commit |
Added SYCL Module Splitting as a library. ESIMD splitting is not present in this patch and will be added in an upcoming patch. Added particular testing tool sycl-module-split that invokes added functionality. Not all `device-code-split` tests were updated in this patch because the rest of them (mostly) don't test module splitting itself. They will be migrated in an upcoming patch.
Add SYCL Module Splitting as a library. ESIMD splitting is not present
in this patch and will be added in the upcoming patch.
Add particular testing tool sycl-module-split that invokes added functionality.
The following list of tests from llvm/test/tools/sycl-post-link/device-code-split additionally invokes new added sycl-module-split tool in order to check the library part.
auto-module-split-1.ll
auto-module-split-2.ll
auto-module-split-3.ll
auto-module-split-func-ptr.ll
basic-module-split.ll
complex-indirect-call-chain.ll
one-kernel-per-module.ll
per-aspect-split-1.ll
per-aspect-split-2.ll
per-aspect-split-3.ll
per-joint-matrix-1.ll
per-joint-matrix-2.ll
per-joint-matrix-mad-1.ll
per-joint-matrix-mad-2.ll
per-joint-matrix-mad-4.ll
per-joint-matrix-mad-5.ll
per-reqd-sub-group-size-split-1.ll
per-reqd-sub-group-size-split-2.ll
per-reqd-wg-size-split-1.ll
per-reqd-wg-size-split-2.ll
split-with-kernel-declarations.ll
vtable.ll
The following list of tests wasn't edited in this patch:
per-joint-matrix-3.ll
per-aspect-split-4.ll
per-joint-matrix-mad-3.ll
per-reqd-wg-size-split-3.ll
split-with-func-ptrs.ll
The above tests aren't involved in this patch because they don't test module splitting
itself. They will be migrated in the upcoming patch.
split-with-func-ptrs.ll requires ESIMD splitting and will be migrated
with the corresponding patch.